Skip to content

test: the last five suites' continuations name the case they continue (#982, the final five of eight) - #992

Merged
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:fix/982-the-last-five-suites-name-their-arm
Sep 12, 2026
Merged

test: the last five suites' continuations name the case they continue (#982, the final five of eight)#992
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:fix/982-the-last-five-suites-name-their-arm

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

This is the last of #982's eight suites — all five of them, because after #984, #989 and
#990 the rule has stopped producing new cases and these ten records are mechanical.

Ten checks across five suites shared five (suite, part, name) keys with another check.

Measured: every suite, both trees, same box

suite control records control keys lost branch records branch keys lost
sorted_pathkeys 113 110 3 113 113 0
vector_agg_tlist_shape 68 65 3 68 68 0
alter_am_cleanup 45 43 2 45 45 0
eager_ordering_record 31 30 1 31 31 0
objstore_userinfo 7 6 1 7 7 0

Control is clean main at 827f2801. Every record count is unchanged, so this renames and
nothing else.
All ten runs: rc=0, FAIL=0.

The two trees ran sequentially, not in parallel, because both worktrees install into the
same pg18a prefix — a concurrent run would have measured the other tree's .so.

This is not a new convention. It is each file's own convention, applied where it lapsed.

That is the part worth checking, and it is why these five needed no new rule. Each file
already names the case at a neighbouring site, then falls back to a bare continuation for
the next several:

test/sorted_pathkeys.sh, as it stands on main:
  and DESC still answers correctly          <- names its case
  and NULLS FIRST still answers correctly   <- names its case
  and it still answers correctly            <- does not           (x2)

test/vector_agg_tlist_shape.sh, as it stands on main:
  and GROUP BY still answers correctly      <- names its case
  and FILTER still answers correctly        <- names its case
  and DISTINCT still answers correctly      <- names its case
  and it still answers correctly            <- does not           (x4)

So the fix is to finish the pattern the author started, taking each discriminator from the
REFUSE: headline directly above it:

REFUSE: a non-prefix of the key is not an order the rows are in
  -> and a non-prefix still answers correctly
REFUSE: a column that is not in the key at all
  -> and a non-key column still answers correctly
REFUSE: a Z-order run is not a sort on its lead column
  -> and the Z-order run still answers correctly
REFUSE: an unsupported aggregate declines the whole target list
  -> and the unsupported aggregate still answers correctly
REFUSE: FILTER inside an expression over aggregates
  -> and FILTER inside an expression still answers correctly
REFUSE: ORDER BY inside an aggregate, wrapped
  -> and a wrapped ORDER BY still answers correctly
REFUSE: an aggregate over an expression of two columns
  -> and an aggregate over an expression still answers correctly

alter_am_cleanup runs the same eight operations in two databases — one without the
extension, one after DROP EXTENSION — so its two pairs take the database from the headline:
and dropped there, without the extension / , after DROP EXTENSION, and the same suffixes
on the REFRESH MATERIALIZED VIEW pair. objstore_userinfo takes the entry point:
and the read_parquet message names userinfo / and the export_parquet message names userinfo.

Two sites take the discriminator from the value expression instead (#990's third form),
because their headline names no table:

control: and it moved the layout   ->  control: and it moved the tailgate layout
                                       control: and it moved the lexgate layout

matching the layout tailgate and layout lexgate those checks actually read.

A check that the renames are removals, not a mistyped grep

Counting exact old names — the phrase followed by its closing quote — across the whole tree:

clean main    17 occurrences
this branch    0 occurrences

and 17 is exactly the number of sites this PR edits, so the two numbers cross-check each
other. Nothing outside the five files mentions any of these names (docs/, CHANGELOG.md,
README.md, .github/ all clean), so no unchanged line is falsified by the renames.

Limits of what this proves

Checks run locally: five suites 264 checks / 0 FAIL on both trees, shellcheck 0 findings,
bash -n clean, docs_style 9/9.

Part of #982 (the last five of eight; #984 merged, #989 and #990 open).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

@OffgridwithJD
OffgridwithJD force-pushed the fix/982-the-last-five-suites-name-their-arm branch from 24b84c7 to d663a51 Compare September 12, 2026 01:46

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at d663a512. Verified with a derived helper list, after my first two attempts under-reported — twice, by two different mechanisms.

What I checked

file                        names      literal collisions
sorted_pathkeys.sh          105 -> 105        1 -> 0
vector_agg_tlist_shape.sh    48 ->  48        3 -> 0
alter_am_cleanup.sh          46 ->  46        2 -> 0
eager_ordering_record.sh     32 ->  32        1 -> 0
objstore_userinfo.sh          8 ->   8        1 -> 0

Name count unchanged in every file, which is the check that says this renames and does not add or remove an assertion.

Both of my earlier sweeps were wrong, in the two directions you would predict

First, I matched only check|check_num|check_skip|ansq|check_timing — a hand-written helper list. sorted_pathkeys emits through ans and ansp, so I saw 82 names where there are 105, and reported 0 -> 0 collisions for a file that has one. A hand-maintained list of what counts as a check, used to audit checks.

The fix is to derive the helpers: every function in the file or lib.sh whose body calls check or pgc_record. That finds ansp, check_ratio, check_text, diff_query_ordered and fifteen more I would never have enumerated.

Second, on #989 the same static method reported 0 literal duplicates on main for a file you measured as 3 colliding. Also correct and also useless: the continuation sits inside a three-iteration loop, so it is one source line and three records.

static source count   under-reports loops (one line, N records)
                      under-reports interpolation (two literals -> one string)
                      over-reports interpolation  (one literal -> N strings)

I predicted the third of those in my #990 review after being caught by it. I then walked into the other two.

The conclusion stands harder than when I wrote it: #983's guard must count over emitted RESULT records. Every static approximation I have tried today has been wrong in a different direction, and I was the one arguing for the runtime form.

#989 separately

objstore_module.sh   names 23 -> 23
                     literal 22 -> 19, interpolated 1 -> 4

Three names move from literal to interpolated, which is precisely the loop fix: the continuation now carries the scheme its headline already had in scope. The count is unchanged, so nothing was added or dropped. Approving that one too.

The rule's four forms, now complete

#984  the continuation carries the discriminator its headline interpolates
#989  a loop's continuation interpolates what the loop already has in scope
#990  where nothing interpolates, it comes from the check's value expression
#992  hand-written sites take a phrase naming the case they continue

Reading the #992 diffs, the pattern that recurs is and it still answers correctly becoming and a non-prefix still answers correctly, and a non-key column still answers correctly, and the Z-order run still answers correctly. Each one is shorter to read than the headline above it and now survives being lifted out of reading order, which was the whole requirement.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased again after #989 merged, so your approval at d663a512 now stands on 7ec8d3c9. Flagging it rather than letting it ride, because an approval surviving a push is exactly what bit me on #985.

What moved: nothing but the rebase. Measured rather than asserted — the content change with CHANGELOG.md excluded, on the approved commit and on the current one:

approved d663a512   165 lines   md5 f49288260f65
current  7ec8d3c9   165 lines   md5 f49288260f65   -> byte-identical

The only difference is that CHANGELOG.md now carries #989's entry above mine, because main does. Both insertions are kept; neither replaced the other.

Same for #993 (af0ca9f8 -> f704817a), which has no approval yet.

If you would rather re-approve on the current head than have the old one carry, say so and I will leave it alone until you do — the #985 lesson was that a review timestamped before a push is not a review of what is there now, even when I can show the content matches.

jdatcmd added a commit that referenced this pull request Sep 12, 2026
@OffgridwithJD's review of #998 found three things, all real, all verified
here before acting on them.

FIVE ARMS VANISHED IN THE FILE THIS PR IS MOST ABOUT. 340 skipped five
arms behind `if [ -z "$_fp_user" ]; then : # already skipped above`.
"Above" skipped the three PREMISES, not these five. Worse, #998 made that
comment more convincing rather than less: a reader auditing the file now
sees a loop above and moves on.

The repair needs `_fp_base=""` before the block -- it is assigned only in
block 1's `else`, and under `set -uo pipefail` letting the `elif` fire
without it kills the suite, which is presumably why the `:` was there.

SIX SITES, NOT FOUR. The two outer pyarrow gates hold their arms in the
`then` branch with the skip in the `else`. A classifier looking only
FORWARD from a skip reads those as armless -- which is how the issue came
to say four, and how my own tool then repeated the same mistake.

THE GUARD COMPARED ONE LOOP PER (FILE, VARIABLE), AND 340 HAS THREE. The
one it compared was the loop #998 had just written both sides of, so it
agreed by construction and examined nothing. Rewritten as a script that
walks every loop.

Two further faults of my own surfaced fixing that:

  a `check_skip "$VAR"` line counted as an ARM, which silently dropped
  four sites from comparison

  python inside a HEREDOC parsed as shell `if`, unbalancing the walk so a
  ten-arm branch reported six -- the tool that finds silently-lost arms
  was silently losing arms

Both fixed; structure is now read from a heredoc-blanked view while names
come from the real lines.

AND THE GAP IS STATED RATHER THAN HIDDEN. 340's five-arm loop -- the one
this change repairs -- is NOT compared, because its sibling arm is
generated by a loop of its own and a literal comparison would be wrong in
both directions. Driven both ways: dropping a name from a comparable loop
is caught, dropping one from that loop is not. The part prints
`interpolated 1` so a total of zero mismatches cannot read as agreement.

    loops 8, compared 6, interpolated 1, armless 1, mismatches 0

Verified: harness_selftest 863/863 on PG16, shellcheck clean at CI's own
flags, docs_style 9/9, ledger 905 -> 909 with the census DERIVED and the
ceiling unchanged.

KNOWN COLLISION: this PR's sorted_pathkeys loop lists `and the whole
ordered result matches heap`, which #992 renames. Whichever lands second
makes part 470 red until the list is updated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
@jdatcmd

jdatcmd commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

The #998 collision is now live on this PR, and here is the one-line fix. #998 merged at 75b3548, so main carries the skip loop that lists this arm's old name.

Composed origin/main + this branch in a worktree and ran the sweep:

merge: clean, NO conflicts
loops 8, compared 6, interpolated 1, armless 1
MISMATCH sorted_pathkeys.sh:283

The loop at 283 lists seven names; the else branch now emits:

  loop lists : "and the whole ordered result matches heap"
  else emits : "and the whole ordered result matches heap under the new collation"

Fix: change that one entry in the loop's list to match the renamed arm. The other six agree.

Note the merge is clean — git has nothing to report, and test/selftest/470 goes red on main only after both are in. That is the collision recorded on both PRs since yesterday, arriving on schedule.

A measurement trap I walked into on the way, worth recording

I first tried to settle this by counting, and got the wrong answer:

grep -cF "and the whole ordered result matches heap"   main 3   pr992 2

and reasoned that since only one of three occurrences changed, the loop's sibling was untouched. Wrong, because grep -F counts a line containing the substring — and both renames here extend the name rather than replace it, so each renamed line still matches. The count could not see the rename at all.

The composed run found it in one command. Counting the name did not, and would have let me tell you there was no collision.

OffgridwithJD and others added 2 commits September 11, 2026 21:09
…commandprompt#982)

Ten checks across five suites shared five ledger keys with another check. Measured
by running all five suites on clean main and on this branch, same box:

    suite                    control (clean main)          this branch
    sorted_pathkeys          113 records 110 keys 3 lost    113 113 0
    vector_agg_tlist_shape    68 records  65 keys 3 lost     68  68 0
    alter_am_cleanup          45 records  43 keys 2 lost     45  45 0
    eager_ordering_record     31 records  30 keys 1 lost     31  31 0
    objstore_userinfo          7 records   6 keys 1 lost      7   7 0

Every record count unchanged, rc=0 and FAIL=0 on both trees, so this renames and
nothing else.

THIS IS NOT A NEW CONVENTION. It is each file's own convention applied where it
lapsed. All five already name the case at a neighbouring site -- "and DESC still
answers correctly", "and NULLS FIRST still answers correctly", "and FILTER still
answers correctly" -- and then fall back to a bare "and it still answers
correctly" for the next several. This finishes the pattern the author started.

Two sites take the discriminator from the value expression instead, because their
headline names no table: "control: and it moved the layout" becomes "... moved the
tailgate layout" and "... moved the lexgate layout", matching the layout tailgate
and layout lexgate the checks read.

Verification beyond the runs: clean main holds 17 occurrences of an exact old
colliding name and this branch holds 0, and 17 is the rename count -- so the zero
is a removal rather than a mistyped grep. No file outside the five mentions any of
these names. Both ledger files are byte-identical to main.

    5 suites 264 checks 0 FAIL on both trees | shellcheck 0 | bash -n clean
    ledger and budget untouched | docs_style 9/9

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…commandprompt#982)

commandprompt#998 landed a skip loop in sorted_pathkeys.sh that lists its sibling arms by name,
and one of those names is the arm this PR renames. The two merge with no conflict,
so nothing announces it -- commandprompt#998's guard simply goes red in main:

    MISMATCH sorted_pathkeys.sh:283
      loop lists : "and the whole ordered result matches heap"
      else emits : "and the whole ordered result matches heap under the new collation"

Six of the seven names agreed, which is why only the guard could see it.

Verified both ways with commandprompt#998's own tool, which is the authority on that number:

    with this commit      loops 8, compared 6, interpolated 1, armless 1, no MISMATCH
    without it            the same counts, plus MISMATCH sorted_pathkeys.sh:283

COUNTING THE OLD NAME IS HOW YOU MISS THIS. An unanchored
`grep -cF 'and the whole ordered result matches heap'` returns 3 on this branch,
because both renames EXTEND the name rather than replace it, so each renamed line
still matches its own old form. The count is structurally blind to the rename it is
being asked about and returns a plausible number rather than an error. Anchored on
the closing quote it returns 1 -- line 289, the loop entry -- and 0 after this fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 12, 2026
…dprompt#994)

Four check_skip calls stood in for 19 named arms under a name none of
those arms has. When the condition failed, those 19 produced no record at
all: a reader could not tell WHICH arms did not run, and the ledger could
not tell a skipped arm from a deleted one, because a skipped arm's row
has no matching record exactly as a removed check's would.

The convention was already in the tree, 30 lines below one of the
offenders: skip under each arm's own name, in a loop.

    340-the-binary-must-be-built-from.sh   3 arms
    native_parquet_flba.sh                 6
    native_parquet_pushdown.sh             3
    sorted_pathkeys.sh                     7

Each site's skip-loop names verified equal, as a set, to the names its
sibling branch emits.

THE ISSUE COUNTED 17. IT IS 19. Two arms in sorted_pathkeys.sh go
through `ansp`, which calls check_text with its first argument, and a
sweep looking for `check` did not see them -- the same hand-written
helper list that cost me a wrong answer reviewing commandprompt#992 an hour earlier.

ONE NAME WAS ALREADY UNSTABLE. `premise: C and $ALTCOLL really disagree
on this data` interpolates the variable whose emptiness CAUSES the skip:

    box with a collation   premise: C and en_US.utf8 really disagree ...
    the skip branch        premise: C and  really disagree ...

so skipping under it would record a key no real run emits, and it is a
different key on every box. The name is now stable and the collation
moves into the display, which is not the key.

AND A GUARD, BECAUSE THE FIX DUPLICATES THE ARM NAMES. A rename in the
sibling branch desynchronises the loop silently, and the skip then
records under a name nothing emits -- the failure this change removes,
reintroduced by an edit nobody thinks is risky. Writing this I put a name
from another open PR's rename into the loop; the set comparison caught it
before it shipped, and commandprompt#982's renames are still landing.

Part 470 sweeps every `for VAR in ... check_skip "$VAR"` loop in the
corpus -- derived, so a fifth site is covered the day it is written -- and
asserts the loop's names equal the sibling branch's arms. The population
is a premise, because a sweep that matched nothing reports the same zero
mismatches as a corpus in agreement.

Removal proof: rename an arm and leave the loop stale -- exactly the
drift made by accident above.

    1 FAIL in the whole suite, and it names the file:
    every skip loop names exactly the arms its sibling branch would emit
      got [sorted_pathkeys.sh] want []

This is the precondition for arming commandprompt#983's orphan guard: until a skipped
arm records under its own name, absence cannot mean removal.

Verified: harness_selftest 862/862 on PG16, shellcheck clean at CI's own
flags, docs_style 9/9. Ledger 905 -> 908, census DERIVED, ceiling 250
unchanged, nothing orphaned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
@OffgridwithJD
OffgridwithJD force-pushed the fix/982-the-last-five-suites-name-their-arm branch from 7ec8d3c to ee2f8f1 Compare September 12, 2026 03:12
@jdatcmd
jdatcmd merged commit c697c8c into commandprompt:main Sep 12, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants